branch-4.1:[opt](variant) Seek unshredded Parquet Variant paths directly - #66758
branch-4.1:[opt](variant) Seek unshredded Parquet Variant paths directly#66758hubgeter wants to merge 10 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: four blocking issues remain in the complete-unshredded Variant direct-seek path.
Findings, ordered by severity:
- The fast path can accept corrupt scalar or container roots that canonical Variant validation rejects, then publishes them through
append_prevalidated. - Selected-subtree validation resets nesting depth and can accept a depth-129 source that the normal Variant contract rejects.
- The unchanged Iceberg p0 suite still requires old path-miss/reconstruction counters that this branch now intentionally eliminates, so reachable regression assertions fail.
- Independent projections of a wide unshredded container each rescan its complete offset table (and re-sort object offsets), creating multiplicative CPU work versus the former cached materialization.
Critical checkpoints:
- Architecture/layering: the implementation remains within core Variant and the v2 Parquet reader/Profile boundary; I found no v1 decoder, Arrow runtime, or table/file identity boundary violation.
- Correctness and external compatibility: selected-only traversal may intentionally avoid unrelated sibling payloads, but it must still preserve exact root-envelope, scalar-domain, and total-depth invariants. The first two inline findings cover the concrete gaps.
- Lifecycle and resource ownership: the metadata views remain owned by the immutable physical state; filter/range/index selections create fresh states, compatible append COW-detaches and invalidates the cache, and retained aliases retain their backing columns. I found no additional lifetime or allocation-accounting defect.
- Schema/materialization/nulls: mixed normalized-unshredded and typed segments preserve segment order, physical primitive identity, SQL null maps, Variant NULL bytes, and exception-before-publication behavior. No additional issue survived review.
- Filtering and deletes: lazy selection, row alignment, page pruning, and position/equality-delete paths were traced. No result-row defect was found; the stale position-delete Profile assertion is included in the third finding.
- Performance and observability: the new persistent counters are wired through the reader Profile, but they do not prevent the repeated wide-container work in the fourth finding, and existing external counter contracts were not updated.
- Tests: the added unit tests cover direct object/array access, appended metadata dictionaries, selected-sibling validation, malformed object tables, and mixed physical segments. They do not cover corrupt root envelopes, the combined depth boundary, or wide multi-projection reuse. Per the review-runner instructions, no build or test command was run; the unchanged p0 assertions were verified statically against the new control flow.
- User focus: no additional user-provided focus was supplied.
The review converged after three full normal and risk-focused rounds. Every candidate is either represented by an inline finding or dismissed with concrete duplicate/lifecycle/semantic evidence; no suspicious point remains unresolved.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one blocking resource/performance regression remains in heterogeneous composite Variant extraction.
Finding:
- A direct unshredded match reused by a heterogeneous composite retains the complete root metadata dictionary in each projected result. The old composite normalization fallback compacted metadata to the selected subtree, so wide dictionaries can now multiply live block memory across projections and make an otherwise valid mixed-file query exceed ColumnString or memory limits.
Critical checkpoints:
- Architecture/layering: the implementation stays within core Variant and the v2 Parquet reader/Profile boundary; I found no v1 decoder, Arrow runtime, table/file identity, or mapper ownership violation.
- Correctness and validation: the current head validates the exact root envelope, every accessed container table, selected-child bounds, and the selected subtree at its root-relative depth. The prior root/depth thread fixes are sound, and no additional corrupt-input acceptance remained.
- Lifecycle and cache ownership: metadata/container views remain backed by immutable physical columns for the cache lifetime. Append and selection paths reset or replace borrowed state under the materialization lock, and entry/depth/promoted-offset budgets are bounded. No additional lifetime or invalidation issue remained.
- Schema/materialization/nulls: homogeneous and heterogeneous segments preserve order, schema-aware primitive identity, outer/leaf null maps, and residual reconstruction fallback. The inline finding is the remaining materialization regression: the value is correct, but its output metadata is no longer compact in mixed composites.
- Filtering and deletes: filter/range/index gathers, lazy predicate scans, page pruning, and Iceberg position-delete alignment preserve row and reconstruction semantics at the reviewed head.
- External compatibility: complete unshredded and projected typed files can legally coexist in the changed Iceberg gather path; that reachable mixed-file state is what exposes the inline finding. No separate Parquet Variant encoding-compatibility defect remained.
- Performance and observability: reusable container lookup state fixes the earlier repeated wide-container scan, but it does not address output metadata copying after a hit.
VariantDirectResidualSeekBytescounts selected value slices and therefore does not expose the retained dictionary cost. - Tests: the changed unit and Iceberg cases cover direct object/array access, malformed roots/tables, depth, cache invalidation/budgets, mixed segment orders, lazy/page/delete paths, and Profile contracts. They verify values and counters but not compact output metadata or allocation. Per the review-runner instructions, no local build or test command was run.
- User focus: no additional user-provided focus was supplied; the whole PR was reviewed.
The review converged in Round 2: every normal and risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS, the one accepted candidate is represented by the inline comment, and no suspicious point remains unresolved.
| // Traversed containers perform bounded reads. Validate the selected subtree exactly, but | ||
| // intentionally do not visit unrelated siblings in the unshredded root. | ||
| validate_variant_payload(current, current_depth); | ||
| selected_rows.push_back(current); |
There was a problem hiding this comment.
[P1] Compact metadata before combining direct residual results
When a heterogeneous composite reuses this direct unshredded result, each selected scalar/miss still carries the root metadata dictionary into append_prevalidated. Before this change, the composite fallback normalized that segment through normalize_materialized_path, whose VariantBatchBuilder retained only keys reachable from the selected subtree. A wide dictionary is now copied into every projected result (and distinct dictionaries are rehashed/copied per path), so E projections retain O(E * root-metadata bytes) and can hit the block/ColumnString limit even after the container lookup cache hits. Please import the selected value through the compacting builder (empty metadata for scalar/missing results and ID remapping for containers), with a wide-metadata mixed-state test.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review result: no new actionable findings. Current head f000266 is acceptable for approval.
Critical checkpoints:
- Architecture and layering: the optimization remains within core Variant and the v2 Parquet reader/Profile boundary. It does not introduce v1 decoder, Arrow runtime, table-schema mapping, or file-local identity leakage.
- Correctness and corruption handling: direct seek validates the exact root envelope and scalar semantics, validates every accessed object/array table even on misses or kind mismatches, preserves root-relative nesting depth, validates selected-child bounds, and recursively validates the selected subtree. Only unrelated sibling payloads are intentionally left unvisited.
- Schema, materialization, and null semantics: homogeneous typed segments retain exact physical identity; heterogeneous typed, projected, unshredded, and reconstruction-only segments retain row order and SQL/Variant null distinctions. Direct results compact metadata to reachable keys, and completed unshredded prefixes are not evaluated twice during composite fallback.
- Lifecycle, memory, and performance: borrowed container state is protected by the materialization lock, reset after compatible append, and replaced by fresh state after filter, cut, or gather. Retention is bounded by the documented 16K-entry, four-ancestor, and 4 MiB promoted-offset limits. The current head also fixes the previously reported wide-metadata amplification by re-encoding only the selected subtree.
- Filtering and external compatibility: predicate-only, lazy materialization, page-pruned, projected-root, mixed-file, warm/cold, and position-delete paths preserve row alignment and reconstruction behavior for Iceberg Parquet Variant data.
- Observability: the direct-seek time, row, byte, fallback, build, and hit counters are initialized and forwarded through the Parquet reader profile. The regression helper now distinguishes counters that must exist from counters that must be positive, so intentional zero reconstruction remains testable.
- Tests: focused coverage exercises object/array paths, null and missing values, malformed roots and container tables, maximum depth, distinct appended metadata, cache reuse/invalidation and caps, noncanonical offsets, mixed segment orders, metadata compaction, lazy/page/delete behavior, and Profile contracts. Per the review-runner instruction, no local build or test command was run.
- Existing review context: all five earlier inline concerns are represented in the current head and were not duplicated. The latest metadata-compaction concern is covered by subtree re-encoding plus CompactsWideMetadataForDirectAndCompositeResults.
- User focus: no additional user-provided focus was supplied; the entire PR was reviewed.
The review converged in Round 1: both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS after their required rechecks, and the main final sweep found no unresolved candidate.
| } | ||
|
|
||
| VariantMetadataRef metadata {.data = metadata_bytes.data, .size = metadata_bytes.size}; | ||
| validate_variant_metadata(metadata); |
| for (size_t position = 0; position < path.size(); ++position) { | ||
| VariantRef selected; | ||
| const VariantBasicType basic_type = | ||
| validate_variant_payload_shallow(current, current_depth); |
There was a problem hiding this comment.
不需要validate， 直接拿basic_type， validate_variant_payload_shallow 这个函数也没必要
| } | ||
|
|
||
| private: | ||
| std::optional<ColumnPtr> find_unshredded_normalized_value( |
There was a problem hiding this comment.
当前实现只优化了“单 path、宽 root”的情况多 path predicate 很容易因为 E × N 次扫描和 leaf 重编码而比原来更慢。
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: two issues remain in the planned unshredded Variant path work.
Findings, ordered by severity:
- The new Variant request-map assertions are attached to a STRUCT-only mapper test. That test cannot populate either Variant map, so its first
map::atdeterministically throws; the assertions belong in the following deferred-Variant test. - A repeated planned-path cache hit recharges the full direct-seek row and selected-byte totals even though it only returns an existing
ColumnPtrand performs no timed seek, validation, re-encoding, or copy.
Critical checkpoints:
- Architecture and layering: logical Variant paths remain in the mapper/request and v2 Parquet reader boundary, are copied into reader-owned plans, and do not leak table-global identities into file-local readers. No v1 decoder, Arrow runtime, or layering defect remained.
- Correctness and external compatibility: accessed container envelopes, complete key/offset tables, selected intervals, negative indices, and successful root-relative depth paths are validated before publication. Remaining scalar/metadata/miss-policy questions are already covered by current live threads and author feedback; I found no distinct corrupt-input or Parquet compatibility issue.
- Lifecycle and resource ownership: metadata, container, and planned-path views borrow immutable physical state. Append clears all borrowed/result caches under the state lock after COW, while filter/range/index operations build fresh states. No stale-view or cache-identity defect remained.
- Schema, materialization, and nulls: mixed typed, projected, and unshredded composite segments preserve segment order, schema-aware primitive identity, SQL/Variant null distinctions, and compact owning results. No additional materialization defect survived review.
- Filtering and deletes: shared and deferred predicate readers receive the intended path union/subset, refreshed requests are reader-scoped, and lazy filtering plus position/equality deletes preserve row alignment. No result-row issue remained.
- Performance and observability: planned retention is bounded by the 64-path and 512K-cell gates, with separate container-entry/depth/promoted-offset caps. Counter construction and Groovy required-versus-positive polling are sound; the second inline finding is the remaining attribution defect.
- Tests: the changed coverage spans routing, malformed selected values, depth, cache invalidation/caps, composites, lazy/page/delete paths, and profiles, but the misplaced assertions make the STRUCT mapper test fail deterministically. Per the review-runner instruction, no build or test command was run.
- Existing review context: neither inline finding duplicates the current root/depth, wide-container, metadata-amplification, or multi-path execution-cost threads.
- User focus: no additional user-provided review focus was supplied; the whole PR was reviewed.
The review converged in Round 2: all three normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS, both accepted findings were independently verified, and no suspicious point remains unresolved.
| EXPECT_EQ(request.local_positions.at(LocalColumnId(0)), LocalIndex(0)); | ||
| EXPECT_EQ(request.non_predicate_position(LocalColumnId(0)), LocalIndex(1)); | ||
| EXPECT_TRUE(request.is_predicate_only(LocalColumnId(0))); | ||
| EXPECT_EQ(request.variant_access_paths.at(LocalColumnId(0)), |
There was a problem hiding this comment.
[P1] Move these Variant assertions into the following Variant test
This test only maps STRUCT<a BIGINT, b BIGINT> and never supplies any Variant access path. register_root_variant_access_paths() explicitly skips non-Variant roots, so both maps are empty and this first .at(LocalColumnId(0)) deterministically throws, making the STRUCT test fail. These typed_col assertions belong in PredicateAccessPathsCreateDeferredVariantRootProjection (after its request assertions), where that path is configured.
| } | ||
| DORIS_CHECK_LE(_physical->size(), | ||
| static_cast<size_t>(std::numeric_limits<int64_t>::max())); | ||
| update_counter(_profile.variant_direct_residual_seek_rows, |
There was a problem hiding this comment.
[P2] Do not charge copied bytes again for a cached path result
When this path already exists in _direct_seek_multi_path_results, the branch only returns the cached ColumnPtr: built_path stays false and neither seek/materialization timer runs. These unconditional updates nevertheless add the full stored byte count (and rows) again, although VariantDirectResidualSeekBytes is documented as bytes copied to the result. Repeated evaluation can therefore inflate reported work arbitrarily while seek time and MultiPathPathRows stay flat—the new repeated-b test already codifies 9 general rows for only 6 built path rows. Gate the work counters on built_path (and use a separate request/cache-hit counter if served-result frequency is useful).
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: three actionable issues remain at b0044d28e991dc171b3b2316d0f39fdb36af7320.
Findings, ordered by severity:
[P1]The direct-seek metadata cache trusts an unvalidated sorted flag. A malformed dictionary can make a present key look absent and return SQL NULL instead of the prior corruption error.[P2]Logical access-path sets are registered only for top-level Variant mappings. Nested Variants below STRUCT/ARRAY/MAP never receive the multi-path decision metadata and retain repeated all-row seek/re-encoding work.[P2]VariantDirectResidualSeekFallbacksmisses complete mixed states reached after an earlier composite segment fails, so the new counter changes with segment order for equivalent reconstruction work.
Checkpoint conclusions:
- Architecture and interfaces: The Scanner/TableReader/ColumnMapper/FileReader ownership direction remains intact, and root predicate/output request maps are rebuilt and localized coherently. The nested mapper-to-recursive-plan boundary is incomplete as described in finding 2.
- Correctness, malformed input, and external compatibility: Selected container bounds, offsets, negative indexes, subtree validation, and Spark/Iceberg Variant shapes were traced. Finding 1 is a concrete corrupt-external-input wrong result. Existing root-envelope and maximum-depth concerns were already raised in live threads and were not duplicated.
- Reader lifecycle and resources: Borrowed metadata/container views remain owned by immutable physical storage; append invalidates caches while holding the materialization lock; filter/cut/index selection creates fresh states; retention and promotion budgets remain bounded. No distinct lifecycle, stale-state, or unbounded-memory defect was found.
- Schema mapping and materialization: Typed and normalized composite paths preserve row order, null maps, and exact physical type identity across append, filter, range, and arbitrary selection. Nested Variant path planning remains the gap in finding 2.
- Filtering, deletes, and pushdown: Shared and independent predicate/output readers, fully filtered batches, equality/position-delete alignment, row-group request rewriting, and deferred output selection were rechecked without another distinct defect.
- Performance and observability: The existing top-level multi-path amplification thread remains live and was duplicate-suppressed. Finding 3 is a separate counter-contract defect in the composite later-fallback branch; other counter creation/export paths are wired.
- Tests: Current tests cover broad direct-seek, composite, mapper, and Iceberg behavior, but do not cover a falsely advertised sorted dictionary, nested request-to-plan propagation, or both composite segment orders with fallback-counter assertions.
Review status: Round 1 produced three accepted findings after main verification and one same-location duplicate. Both normal Round 2 rechecks and the separate risk recheck returned NO_NEW_VALUABLE_FINDINGS, so the review converged in Round 2. The live head/base and review context were refreshed immediately before submission. No additional user-provided focus was present. Per the review environment instructions, this was a static review and no build or test command was run.
| // cache therefore only deduplicates the immutable metadata bytes instead of eagerly | ||
| // walking every dictionary key. The bounded find_key()/key_at() accessors still reject | ||
| // truncated layouts before any referenced key is observed. | ||
| VariantMetadataRef metadata {.data = metadata_bytes.data, .size = metadata_bytes.size}; |
There was a problem hiding this comment.
[P1] Validate metadata before trusting the sorted flag
This cache stores each unique metadata blob without validation, but path preparation immediately calls find_key(), which binary-searches whenever the header merely advertises sorted strings. For example, metadata keys ['z','a'] marked sorted plus a root object id table [1,0] is locally ordered as a,z, yet lookup of z returns -1 and this path emits SQL NULL even though the key is present. The previous materialization path rejected the same row through validate_variant_metadata(). Please validate each unique dictionary before using its ordering/uniqueness claims and cover a falsely advertised sorted dictionary. This is distinct from the existing root-payload/depth threads because the wrong result comes from the metadata search invariant.
| bool is_predicate_column) { | ||
| DORIS_CHECK(file_request != nullptr); | ||
| if (!mapping.file_local_id.has_value() || mapping.table_type == nullptr || | ||
| mapping.variant_access_paths.empty() || |
There was a problem hiding this comment.
[P2] Carry access paths to nested Variant terminals
Nested Variant paths are preserved on recursive child mappings, but this root-only registration returns for a surrounding STRUCT/ARRAY/MAP, and recursive build_variant_plan() calls receive no path metadata. A nested unshredded Variant therefore sees an empty plan and bypasses the new multi-path materialization fallback, so queries such as info.payload['x'] plus info.payload['deep']['name'] still seek and re-encode every row once per expression. Please represent/pass paths per nested Variant terminal and add STRUCT/ARRAY/MAP multi-path coverage. This is distinct from the live top-level multi-path thread because that fix is never wired to nested terminals.
| // requested fallback. In particular, do not seek an unshredded segment twice. | ||
| normalized = matches[index].normalized; | ||
| } else { | ||
| normalized = _segments[index]->find_normalized_value(path); |
There was a problem hiding this comment.
[P2] Count mixed residual fallbacks reached through normalization
After an earlier segment misses, this loop sends every unvisited segment directly to find_normalized_value(). A later complete mixed Parquet segment then reconstructs through materialized_column(), but VariantDirectResidualSeekFallbacks is incremented only by that segment's skipped find_typed_value() fallback. Consequently [partial miss, complete mixed] reports zero while the reversed segment order reports one for the same reconstruction work, contrary to the new counter contract. Please count the complete-mixed-to-materialized transition in one common place and test both segment orders. This is separate from the old cached rows/bytes comment because it concerns the residual fallback counter and composite branch.
A selected Variant path used to be re-encoded into owning Variant bytes and then parsed again per row by whatever consumed it. For a wide unshredded root projected by many paths that cost more than importing the root once, which is why two or more unique paths were routed back to a full materialization. Collect the selected value through VariantSelectedValueBuilder instead. A homogeneous scalar projection - the shape every constant path over a JSON-like root produces - becomes a typed ColumnVariantV2, so the leaf is decoded once and a CAST reads a plain Doris column. Containers, mixed scalar kinds, kinds with no exact Doris type, and integers written wider than their value needs degrade to canonical encoded rows by replaying the values already collected, so no result changes and no source is revisited. Integers keep their observable Variant type because a typed BIGINT column re-encodes through the narrowest signed width. With the owning rebuild gone the multi-path guard has no reason to exist, so every unique path direct seeks again. A string typed identity also short circuits the STRING cast, which now reuses the column instead of rebuilding a scalar view per row. Also: - Validate each distinct unshredded metadata dictionary once while it is cached. find_key() binary searches whenever the dictionary claims sorted_strings, so a dictionary that sets that flag while storing unordered keys would silently turn a present key into SQL NULL. - Count the mixed residual fallback once per state, at the path-request boundary shared by find_typed_value() and find_normalized_value(). The total no longer depends on which composite segment observed the miss first, and whole-column serialization is no longer counted as a seek fallback. - Add VariantDirectResidualSeekTypedRows and VariantDirectResidualSeekTypedDowngrades so a Profile shows whether a path actually reached the typed path.
Resolved four conflicts, all against apache#66744 "Optimize Variant V2 ingestion and STRING casts" and its VariantRef::ObjectView work: - variant_value.{h,cpp}: kept both additions. _container_offset() and the index_out output of _object_find_by_id() belong to the direct-seek container lookup on this branch; object_view() and the optional dictionary_size of _object_field_id() come from upstream. They do not overlap, and passing no dictionary_size keeps the previous metadata.dict_size() behaviour. - cast_variant_to_string.cpp: upstream's payload-reuse short circuit supersedes the one added here, because it also merges forced nulls without rebuilding the column. Kept it, and kept this branch's handling of the remaining case, where a string typed column does contain Variant nulls and the surviving rows can still be read straight from the typed payload. - cast_variant_v2_from_test.cpp: kept upstream's typed_strings() fixture and its two tests, which cover payload reuse and the forced/inner null matrix more precisely. Kept only the added test that cross-checks the typed and encoded casts byte for byte, which is the invariant typed direct-seek results rely on.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for one actionable Profile accounting defect in the complete-unshredded Variant direct-seek path.
Finding:
VariantDirectResidualSeekBytesmixes source encoded slice sizes, synthetic miss bytes, and typed/compacted result representations, so it does not measure its documented output-copy byte unit even on the first successful seek.
Critical checkpoints:
- Architecture and mapping: Scanner/TableReader/ColumnMapper/FileReader ownership remains intact. Shared versus deferred predicate/output readers receive the intended root path union/subset. The existing top-level multi-path and nested-Variant concerns were duplicate-suppressed against live threads.
- Correctness and malformed input: selected container bounds, root-relative depth, typed/encoded value equivalence, SQL versus Variant NULLs, primitive widths, composite order, and corruption propagation were traced. No distinct result-corruption issue remains beyond live threads.
- Lifecycle and resources: borrowed metadata/container state remains backed by immutable physical columns, guarded by the materialization lock, reset on append, replaced on selection-derived states, and bounded by entry/depth/promotion caps.
- Filtering and external compatibility: predicate/lazy-output alignment, Iceberg equality/position deletes, mixed files, and Profile forwarding remain coherent; no new table-format issue was found.
- Performance and observability: the inline byte-unit finding is the remaining new issue. The all-missing typed-downgrade and failed-seek cache-attribution candidates were dismissed after final-round reachability/result-shape checks.
- Tests: coverage is broad, but exact integer/string/missing/container assertions for the byte unit are absent. Per the review-run constraints, no build or test command was run.
- User focus: no additional user-provided focus was supplied.
Review status: converged in Round 3. All three normal and both risk-focused final-round agents returned NO_NEW_VALUABLE_FINDINGS; every candidate was accepted, duplicate-suppressed, or dismissed with concrete evidence.
| // remapped ids; copying the root dictionary would multiply wide metadata across | ||
| // independently projected paths and composite states. | ||
| builder.append_selected(current); | ||
| add_selected_bytes(current.value.size); |
There was a problem hiding this comment.
[P2] Keep direct-seek bytes in one documented unit
VariantDirectResidualSeekBytes is documented as selected encoded bytes copied to the result, but this adds the source current.value.size even when VariantSelectedValueBuilder decodes a homogeneous scalar into a typed Doris column without copying that slice. Missing rows also add a synthetic one-byte null even though they are represented by the result null map, while container/mixed batches rebuild compact bytes whose size can differ from the source. The counter therefore mixes incompatible units on the first successful request, independently of the older cached-result thread. Please either define/count source encoded bytes examined (with misses contributing zero) or measure the actual output bytes, and add exact integer/string/missing/container assertions.
A projection over many paths looked the row's root object up in the keyed container cache once per path. Every one of those lookups hashed the same four borrowed pointers and found the same entry: the last unshredded benchmark run reported 8.82B cache hits against 180M builds for the fifty-path query, and 2.52B against 1.26B for the three-path dashboard. The root of a row is fully determined by the row, because the decoded physical column is immutable until the state appends or resets. Retain roots in a row-indexed array instead, so a wide projection parses each root once and every later path reaches it by subscript. Deeper containers keep the keyed cache, which still dedupes values that repeat across rows. Retained state stays bounded: the array holds at most as many roots as the existing keyed cap allows entries, and rows past it fall back to that cache. Build and hit accounting is unchanged, so a batch still reports one build per distinct container and one hit per reuse.
Building a container lookup validated an object's key order by resolving every field's key through the metadata dictionary and comparing the bytes, and the field search then reparsed the metadata header on every probe. Both costs scale with the width of the root object and are paid per row, so a narrow projection over a wide root spends far more on structural validation than on the value it selects: the last benchmark run reported one container build per row for every query, which a fifty-path projection amortizes and a single-path one does not. A dictionary that declares sorted_strings stores its keys in lexicographic order, so field ids order an object's fields exactly as their keys do. Compare ids in that case, which keeps the same rejection for an object whose fields are not strictly ordered while dropping a dictionary lookup and a key comparison per field. The direct-seek metadata cache is the only caller that builds these lookups and it already validates that declaration, and every key offset behind it, once per distinct dictionary. Also hoist the sorted flag, the dictionary size, and the unsorted search's target key out of the field search, so a probe no longer reparses the metadata header at every step, and resolve the target key only on the path that compares key bytes. Rejecting an out-of-range field id becomes explicit because the sorted search no longer resolves that key on its way past. Unsorted dictionaries keep comparing key bytes, now covered by a direct-seek test whose dictionary stores its keys out of order.
|
run buildall |
|
/review |
Validating a container's lookup tables read every entry through read_unsigned(), which takes its width as a run-time argument and therefore loops over the bytes of each entry. An object pays two of those reads per field on every row, and the benchmark builds one container lookup per row for every query, so a root with fifty fields repeats that loop a hundred times per row no matter how few paths the query projects. A container's id and offset tables use one width for every entry, so resolve it once and validate through a fixed-width read. The checks, their order, and the exceptions they raise are unchanged; only the width becomes a compile-time constant, which collapses each read to a single load. This matters most where the per-row cost cannot be amortized. Solving the last benchmark run for its two components gives roughly 52ns per row against 4ns per additional projected path, so a single-path query spends over ninety percent of its time on work that a fifty-path projection spreads across fifty values. The container layout already proves both tables lie inside the value and that every width is one to four bytes, so reading them directly keeps the same bounds. Arrays get the same treatment for their offset table.
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
3 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)